Conversation
rustbot
added
S-waiting-on-review
Collaborator
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
asder8215
force-pushed
the
components_rewrite
branch
from
May 12, 2026 09:09
1627e2f to
33e69e1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
asder8215
force-pushed
the
components_rewrite
branch
from
May 12, 2026 17:05
33e69e1 to
ed9d33d
Compare
This comment has been minimized.
This comment has been minimized.
asder8215
force-pushed
the
components_rewrite
branch
from
May 12, 2026 17:19
ed9d33d to
0b0f84c
Compare
This comment has been minimized.
This comment has been minimized.
asder8215
force-pushed
the
components_rewrite
branch
from
May 12, 2026 22:05
0b0f84c to
8ed33ea
Compare
This comment was marked as outdated.
This comment was marked as outdated.
asder8215
force-pushed
the
components_rewrite
branch
from
May 13, 2026 22:21
2151b8f to
83cdbed
Compare
This comment was marked as outdated.
This comment was marked as outdated.
asder8215
force-pushed
the
components_rewrite
branch
from
May 15, 2026 00:30
83cdbed to
3921fff
Compare
asder8215
marked this pull request as draft
May 16, 2026 12:22
rustbot
added
S-waiting-on-author
asder8215
force-pushed
the
components_rewrite
branch
from
May 17, 2026 16:09
0a25dda to
92e0132
Compare
Contributor
Author
|
New benchmarking results. You can see what the benchmark code looks like here and run it yourself to see if there are any difference in measurements on your end: This is the measurement of the current implementation of This is the measurement of the new implementation of Edit: Updated |
Contributor
Author
|
Here are the benchmark results with black box: From current From this Edit: Updated Edit 2: Took off Path ordering benchmark here since it was incorrect see below to see corrected path ordering benchmarks. |
asder8215
force-pushed
the
components_rewrite
branch
from
May 17, 2026 18:41
92e0132 to
574d7f2
Compare
asder8215
marked this pull request as ready for review
May 17, 2026 18:59
rustbot
added
S-waiting-on-review
Contributor
Author
|
I'm confident this code works (passed CI in previous run, the current amended commit change I made doesn't change logic, but makes the code written in a more idiomatic way). In my opinion, the logic in this code should look more readable than how |
Contributor
Author
|
@rustbot label +I-libs-nominated Since |
rustbot
added
the
I-libs-nominated
Contributor
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (6e44b7b): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.2%, secondary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (primary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 474.853s -> 473.232s (-0.34%) |
rustbot
removed
the
S-waiting-on-perf
Contributor
|
Yeah, that's expectedly unhelpful, probably because the standard library is compiled in release mode. I'm not actually sure you can easily run the debug versions without build-std or testing the repo? Will have to double-check, but pretty sure this should be fine. If you want to try running your benchmarks with debug mode combined with the release-compiled std, pretty sure the rustc dev guide has some notes on making rustup use a custom libstd build. |
Contributor
Author
Forgot to respond, but I can try for that. I'll run the try command to build rustc with PGO optimizations and then use Hope you have a good time at RustConf! @bors try |
Contributor
|
⌛ Trying commit d0b7c95 with merge 406b1fe… To cancel the try build, run the command Workflow: https://github.com/rust-lang/rust/actions/runs/34134782370 |
rust-bors Bot
pushed a commit
that referenced
this pull request
Sep 7, 2026
Split `Components` iterator to prefixed and non-prefixed versions and optimize `Components` for non-prefix path based platforms
Contributor
Author
|
Oh wait, I can just install the build you created from your earlier try @bors try cancel |
Contributor
|
Try build cancelled. Cancelled workflows: Hint: if you want to run another try build, you do not need to manually cancel the previous one. Just run |
Contributor
Author
|
So good news, the release compiled std in debug outperforms the Criterion Benchmark Results — Component (Debug) vs Release Compiled Std Components (Debug)Times shown are the Criterion point estimate (middle of the confidence interval). Component was built in debug mode; Std Components was compiled in release mode but benchmarked under the debug harness/profile. Rel Short Path with 1 byte comps
Abs Short Path with 1 byte comps
Rel Short Path with 3 byte comps
Abs Short Path with 3 byte comps
Rel Short Path with 7 byte comps
Abs Short Path with 7 byte comps
Rel Short Path with 15 byte comps
Abs Short Path with 15 byte comps
Rel Short Path with 31 byte comps
Abs Short Path with 31 byte comps
Rel Short Path with 63 byte comps
Abs Short Path with 63 byte comps
Rel Short Path with 127 byte comps
Abs Short Path with 127 byte comps
Rel Short Path with 255 byte comps
Abs Short Path with 255 byte comps
Rel Long Path with 1 byte comps
Abs Long Path with 1 byte comps
Rel Long Path with 3 byte comps
Abs Long Path with 3 byte comps
Rel Long Path with 7 byte comps
Abs Long Path with 7 byte comps
Rel Long Path with 15 byte comps
Abs Long Path with 15 byte comps
Rel Long Path with 31 byte comps
Abs Long Path with 31 byte comps
Rel Long Path with 63 byte comps
Abs Long Path with 63 byte comps
Rel Long Path with 127 byte comps
Abs Long Path with 127 byte comps
Rel Long Path with 255 byte comps
Abs Long Path with 255 byte comps
Rel Long Path Inconsistent Comp
Abs Long Path Inconsistent Comp
I think we shouldn't feel too concerned about perf regression on debug mode then! :D |
This comment has been minimized.
This comment has been minimized.
… of mutating and subslicing path field; as a result, Components iterator memory size goes from 64 bytes to 40 bytes and as_path does not use cloning at all
…ity, added safety comments, and check for root dir after Prefix component (e.g., '\\?\checkout\src\tools' should produce Prefix, RootDir, Normal, Normal, None, ...) in Components::parse_single_component
…ng iter().position()/rposition()
…here to use iter().position()/.iter().rposition(), refactored code in compare_components, and removed stale comments
…omponents::normalize_back instead, refactored Components::as_path code
…e in previous implementation, but making it work with Components<'_> front index
…nt_front and consume_first_component_back. Also introduced aggressive inlining.
…comparison (normalizing paths if needed) and return Ordering Equal/Greater/Less if possible before needing to fall back on Iterator::cmp
… make it simpler or have it documented in the appropriate areas, and renamed a couple of methods/field members, and simplified Components::next_back relative first component branch
…yte equality and Iterator::eq directly. Co-authored-by: =?UTF-8?q?=E7=A6=BE=E5=8F=AF?= <chengkelfan@qq.com>
…path check to Components/Path equality by finding first mismatched byte, separated Components iterator into different versions and re-exported them accordingly based on target os. This also ends up removing HAS_PREFIXES because it becomes unused.
…rtialOrd implementation for non-prefixed Components via directly comparing mismatched byte if it's not a separator byte or cur dir byte; refactor while loop code to use iter().enumerate().find() instead
…e conditional style code rather than match cases (degrades Components::next_back for some reason)
…zes away trailing separator bytes and non-starting current directory component a well as added Components::is_sep_byte for nonprefixed_path.rs
asder8215
force-pushed
the
components_rewrite
branch
from
September 16, 2026 05:14
d0b7c95 to
d5bfd16
Compare
Collaborator
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Contributor
Author
|
Didn't introduce any significant change on rebase. I just updated this branch to the main one and added Other than that, I added clarifying documentation to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
This PR entirely changes how
Components<'_>is implemented.Currently, theThisComponents<'_>iterator 'consumes' components through mutating its path field to a subslice that presents the left over unconsumed path components (this consumed path component is what's returned inComponents::nextorComponents::next_back). However, this PR keeps the path field alive/unmodified and uses front and back indexing strategy to extract consumed/unconsumed components.Components<'_>implementation still uses a subslicing approach similar to the original implementation, but it's highly optimized for non-Windows platform, given that there's noPrefix<'_>components to check with.Ideally, at least from a few benchmarking test I've done locally, this
Components<'_>implementation should perform similarly with the original one when it comes toComponents::next/Componentsordering comparisons (the original one was already highly optimized as is). Where this non-Windows implementation shine though is improvingComponents::as_path(as a result certain things benefit from this likeAncestorsiterator,std::fs::create_dir_all, etc.) andComponents::next_back(and as a resultPathequality).There are a lot of different things I've done to make sure that
Componentsequality is improved dramatically. For example, just like whatComponentscomparison does with checking if we see a mismatched byte in the forward direction and setting ourComponentsiterator to the nearest separator, we do the same thing withComponentsequality in the opposite direction (this actually is one of the significant reason why this implementation ofComponentsequality is really, really faster now). Additionally, there are other optimizations I do here, like doing less boolean checks or usize comparisons withinComponents::next_backand normalizing any separator/curr directory bytes upfront (the original implementation had a lot of conditional checks that could be way more simplified; the prefix checking also didn't benefit non-Windows platform). With the way I haveStateenum implemented in thisComponentsiterator, I can do other small optimizations like checking for empty paths, so that I can returntrueorfalseimmediately.Because the implementation I'm going for is specific to non-Windows/non-prefix-supporting platforms, I chose to split the
Componentsiterator implementation to two different version that is re-exported accordingly to individual platforms (this leads toHAS_PREFIXESconstant being redundant and unnecessary). I'm open to discussion on how to organize the code here better, naming stuff, etc. Some prefix-specific things have been refactored to be exclusively on platforms that utilize prefix components.So far, I've been benchmarking it locally. I formerly did it with Criterion, but now I've just been benchmarking manually through hyperfine. What I've been testing was how fast can this implementation of
Components::next/Components::next_backrun paths like "a1..aN/a1..aN" vs "b/a1..aN/a1..aN/" (where N is some number of a's I put for each component, and you can assume that there are some X path components in the path); I also played around with the mismatching component placed at the end of the path versus beginning, and benchmarked running the same path equality/comparisons 10000 times to see the result cumulatively. You can try hyperfining the code in this repo; I'll get around to consolidating those benchmarks in Criterion a bit later and then posting results in the comments below.